Skip to content

stream: eos premature close#28720

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-pipeline-premature
Closed

stream: eos premature close#28720
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-pipeline-premature

Conversation

@ronag

@ronag ronag commented Jul 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

For writable premature close is close before finish, i.e. it should look for finished instead of ended. Would be better with a finishEmitted but we don't have that.

For readable we should be checking endEmitted (which we do first but not later).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag

ronag commented Jul 16, 2019

Copy link
Copy Markdown
Member Author

I'm unsure how to make tests that actually trigger this... but the semantics I think are more correct like this

@ronag ronag force-pushed the stream-pipeline-premature branch from 554b8e0 to 291d05c Compare July 16, 2019 18:12
@ronag

ronag commented Jul 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

This actually fixes a bug (close before end should be an error). Though it should probably be sem-major due to the updated test.

@ronag

ronag commented Jul 17, 2019

Copy link
Copy Markdown
Member Author

@benjamingr who's a good ping on this one?

@benjamingr

benjamingr commented Jul 18, 2019

Copy link
Copy Markdown
Member

I don't mind reviewing it - can you explain the tradeoffs in the new behavior vs. the existing one?

@ronag

ronag commented Jul 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@benjamincburns close before finish (writable) and close before end (readable) should be premature close error. Which it isn't in every case before this PR (see test update).

@benjamingr

benjamingr commented Jul 18, 2019

Copy link
Copy Markdown
Member

@nodejs/streams

benjamingr
benjamingr approved these changes Jul 18, 2019
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a green citgm run

benjamingr
benjamingr reviewed Jul 18, 2019
View reviewed changes
Comment thread test/parallel/test-stream-finished.js Outdated
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 18, 2019
@mcollina

mcollina commented Jul 18, 2019

Copy link
Copy Markdown
Member

Marking it as semver-major as it changes behavior.

@mcollina

mcollina commented Jul 18, 2019

Copy link
Copy Markdown
Member

What is the bug this is fixing? What’s the goal of this change?

mcollina
mcollina requested changes Jul 18, 2019
View reviewed changes
Comment thread test/parallel/test-stream-finished.js Outdated
@ronag

ronag commented Jul 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mcollina

What is the bug this is fixing? What’s the goal of this change?

close before finish (writable) and close before end (readable) should be premature close error. Which it isn't in every case before this PR (see test update).

@ronag ronag force-pushed the stream-pipeline-premature branch 2 times, most recently from afd02b1 to 1ee52d4 Compare July 18, 2019 11:23
@benjamingr

benjamingr commented Jul 18, 2019

Copy link
Copy Markdown
Member

@ronag just to elaborate on Matteo's comment regarding semverness - changes to the order of the events streams (or in general node APIs) fire are pretty substantial changes with possible big implications on the ecosystem so generally they're all semver-major (means they'll be released in the next major release - in this case 13)

@ronag

ronag commented Jul 18, 2019

Copy link
Copy Markdown
Member Author

@benjamingr no problem, makes perfect sense

This was referenced Aug 6, 2019
Closed
Closed
@ronag ronag force-pushed the stream-pipeline-premature branch from 1ee52d4 to 77ff0d3 Compare August 9, 2019 16:06
@ronag

ronag commented Aug 28, 2019

Copy link
Copy Markdown
Member Author

updated test

@ronag ronag mentioned this pull request Aug 28, 2019
Closed
4 tasks
@ronag

ronag commented Sep 1, 2019

Copy link
Copy Markdown
Member Author

merged into #28748

@ronag ronag closed this Sep 1, 2019
ronag added a commit to nxtedition/node that referenced this pull request Sep 2, 2019
@ronag
fixup: merge nodejs#28720
5641590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina requested changes

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ronag @benjamingr @mcollina